feat: Add profile-specific configuration for disallowed methods and types#15779
Conversation
|
@blyxyas ping |
|
Hi @Lallapallooza, |
There was a problem hiding this comment.
I'll recount what I understand here, so a user uses a special configuration for settings profiles in their clippy.toml so any function can opt-in to those profiles, enabling warnings to those designated disallows in that function.
For example, one might want to disallow functions that may execute arbitrary code from a string input, into user-input-handling functions, and this is a way to achieve via Clippy warning of those uses.
Am I correct, have I forgotten/misunderstood something? If that's the case, we might be interested in generalizing this a bit so that both (1.) we can use profiles for other lints without the need of adding a special attribute for each lint, and (2.) people can use the general idea of profiles in Clippy to segment their source code for several lints at the same time.
Also, @ojeda could this be interesting for the Rust4Linux project? Should I give it priority?
|
You read it correctly. This PR introduces profile-scoped disallow lists in I agree that generalizing this is a good idea. I can follow up with a patch if that would be useful. Do you have any preferences for the design or the user interface (i.e., how Clippy users would enable and use profiles)? |
|
Thanks @blyxyas for the ping! It is an interesting idea. IIUC, currently this can only be done at a crate granularity, right? So it is essentially a way to make Clippy more granular -- I agree that generalizing it makes sense. I will try to think if we could use it in Rust for Linux (@Lallapallooza What are the use cases that motivated this? It may help to give some in the docs to inspire others -- thanks!). |
|
Hi @ojeda. My initial contribution was motivated by a need in my project to disallow host-side operations (e.g., any conversion from a device tensor to a host tensor) in specific context (e.g. dir crates/lib/cuda/) so that such code fails to compile. |
I'll ask both the entire team, and the style team to see if they have anything to say. In Clippy we don't really have an in-house attribute style guide, so maybe they have some feedback about how this could be implemented. |
|
Thanks @Lallapallooza, that is good to know (if you have more cases you can think of, then it would be great to hear about them, of course). |
This comment has been minimized.
This comment has been minimized.
|
Hi Lallapallooza, I've talked with the team and I'm going to proceed with the review. Also, sorry for the delayed response, I've just returned from vacation. I was thinking that this may be useful for this message thread
I thought that we already had a configuration option for this, but it seems that we don't have one. So I think that this feature can also support this use case. |
a8c3a91 to
0ff61e7
Compare
This comment has been minimized.
This comment has been minimized.
|
@blyxyas Thanks, I have rebased pr. |
I'm walking back this statement, we already have another pull request taking care of this. So we won't focus on that front here. |
|
Reminder, once the PR becomes ready for a review, use |
|
Hi! I have addressed all comments, could you please check again. |
| pub fn active_profiles(&mut self, cx: &LateContext<'_>, hir_id: HirId) -> Option<&ProfileSelection> { | ||
| if self.cache.contains_key(&hir_id) { | ||
| return self.cache.get(&hir_id).and_then(|selection| selection.as_ref()); | ||
| } |
There was a problem hiding this comment.
self.cache.contains_key is only self.cache.get().is_some(), so it's performing the same lookup two times. It should be something more akin to:
if let Some(selection) = self.cache.get(&hir_id) {
return selection;
}
There was a problem hiding this comment.
added a NOTE why it's needed
| std::mem::drop(value); //~ ERROR: use of a disallowed method `std::mem::drop` | ||
| } | ||
|
|
||
| #[expect(clippy::disallowed_methods)] |
There was a problem hiding this comment.
We also need expects for the unknown_profile lint, and trying it before and after the profile attribute.
There was a problem hiding this comment.
Added tests in both disallowed_profiles_methods and disallowed_profiles_types covering #[expect(...)] placed before and after #[clippy::disallowed_profile("unknown_...")].
| profiles: FxHashMap<Symbol, TypeLookup>, | ||
| known_profiles: FxHashSet<Symbol>, |
There was a problem hiding this comment.
Why is profiles different from known_profiles? Is there a difference between these two? Seems that known_profiles is just a set from the keys of profiles.
There was a problem hiding this comment.
They're intentionally different: known_profiles holds every name declared in [profiles.*], while profiles only holds those that contribute entries to this lint.
The distinction is what suppresses an "unknown profile" warning when a profile only defines entries for the other lint.
Added field-level comments to make this explicit. Thanks
| let mut active_profiles = SmallVec::<[Symbol; 2]>::new(); | ||
| let mut unknown_profiles = SmallVec::<[ProfileEntry; 2]>::new(); | ||
| if let Some(selection) = self.profile_cache.active_profiles(cx, hir_id) { | ||
| for entry in selection.iter() { | ||
| if self.profiles.contains_key(&entry.name) { | ||
| active_profiles.push(entry.name); | ||
| } else if !self.known_profiles.contains(&entry.name) { | ||
| unknown_profiles.push(*entry); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for entry in unknown_profiles { | ||
| self.warn_unknown_profile(cx, &entry); | ||
| } |
There was a problem hiding this comment.
If we are going to lint them one by one anyways, this self.warn_unknown_profile should be above.
| let mut active_profiles = SmallVec::<[Symbol; 2]>::new(); | |
| let mut unknown_profiles = SmallVec::<[ProfileEntry; 2]>::new(); | |
| if let Some(selection) = self.profile_cache.active_profiles(cx, hir_id) { | |
| for entry in selection.iter() { | |
| if self.profiles.contains_key(&entry.name) { | |
| active_profiles.push(entry.name); | |
| } else if !self.known_profiles.contains(&entry.name) { | |
| unknown_profiles.push(*entry); | |
| } | |
| } | |
| } | |
| for entry in unknown_profiles { | |
| self.warn_unknown_profile(cx, &entry); | |
| } | |
| let mut active_profiles = SmallVec::<[Symbol; 2]>::new(); | |
| let mut unknown_profiles = SmallVec::<[ProfileEntry; 2]>::new(); | |
| if let Some(selection) = self.profile_cache.active_profiles(cx, hir_id) { | |
| for entry in selection.iter() { | |
| if self.profiles.contains_key(&entry.name) { | |
| active_profiles.push(entry.name); | |
| } else { | |
| self.warn_unknown_profile(cx, &entry); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Done, thanks! Dropped the intermediate unknown_profiles SmallVec and warn inline.
I had to copy entries out of the cache first because warn_unknown_profile takes &mut self and the active_profiles borrow holds &mut self.profile_cache.
| use rustc_span::{Span, Symbol}; | ||
|
|
||
| #[derive(Copy, Clone)] | ||
| pub struct ProfileEntry { |
There was a problem hiding this comment.
We'll need some documentation here, what is the entry for? What does it represent?
There was a problem hiding this comment.
Added a doc comment explaining that each entry represents one profile-name argument from a #[clippy::disallowed_profile(s)] attribute. Thanks!
| if let Some(selection) = self.profile_cache.active_profiles(cx, hir_id) { | ||
| for entry in selection.iter() { | ||
| if self.profiles.contains_key(&entry.name) { | ||
| active_profiles.push(entry.name); | ||
| } else if !self.known_profiles.contains(&entry.name) { | ||
| unknown_profiles.push(*entry); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if let Some(selection) = self.profile_cache.active_profiles(cx, hir_id) { | |
| for entry in selection.iter() { | |
| if self.profiles.contains_key(&entry.name) { | |
| active_profiles.push(entry.name); | |
| } else if !self.known_profiles.contains(&entry.name) { | |
| unknown_profiles.push(*entry); | |
| } | |
| } | |
| } | |
| if let Some(selection) = self.profile_cache.active_profiles(cx, hir_id) { | |
| active_profiles = selection.iter().filter(|profile| { | |
| if !self.profiles.contains_key(&profile.name) { self.warn_unknown_profile(cx, &profile); false}; true | |
| }) | |
| } |
There was a problem hiding this comment.
Applied the inline-warn refactor from the sibling thread. Kept the known_profiles guard so that profiles used for the other lint don't get flagged as unknown here.
This comment has been minimized.
This comment has been minimized.
d7e5a4b to
647bc06
Compare
This comment has been minimized.
This comment has been minimized.
|
Hi @blyxyas, sorry for leaving this PR sitting for so long. I've rebased onto current master and addressed all the outstanding review comments (docs on Replies are in each thread. Whenever you have a moment, could you take another look? Thanks again for the reviews. 🙏 |
|
@blyxyas Hi, could you check my patch again please. |
This comment has been minimized.
This comment has been minimized.
- document `ProfileEntry` and `ProfileSelection` - explain why `profiles` and `known_profiles` are separate - emit unknown-profile warning inline (drop intermediate SmallVec) - add tests covering `#[expect]` before/after `#[clippy::disallowed_profile]` with an unknown profile name, for both methods and types lints - sort_by -> sort_by_key to satisfy dogfood
647bc06 to
03128d7
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@blyxyas Thanks! I rebased MR and now it's mergeable. |
View all comments
Add profile-scoped disallow lists for methods and types, wiring the new configuration tables through a shared resolver that can be toggled with #[clippy::disallowed_profile] attributes.
changelog: [disallowed_methods]: allow selecting per-scope disallow lists via disallowed-methods-profiles and the clippy::disallowed_profile attribute
changelog: [disallowed_types]: allow selecting per-scope disallow lists via disallowed-types-profiles and the clippy::disallowed_profile attribute